Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v18.x backport] child_process: improve spawn performance on Linux #50098

Closed

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Oct 9, 2023

Backport of #48523.

Original commit message:

    [base] add build flag to use MADV_DONTFORK

    Embedders like Node.js and Electron expose fork(2)/execve(2) to their
    users. Unfortunately when the V8 heap is very large, these APIs become
    rather slow on Linux, due to the kernel needing to do all the
    bookkeeping for the forked process (in clone's dup_mmap and execve's
    exec_mmap). Of course, this is useless because the forked child thread
    will never actually need to access the V8 heap.

    Add a new build flag v8_enable_private_mapping_fork_optimization which
    marks all pages allocated by OS::Allocate as MADV_DONTFORK. This
    improves the performance of Node.js's fork/execve combination by 10x on
    a 600 MB heap.

    Fixed: v8:7381
    Change-Id: Ib649f774d4a932b41886313ce89acc369923699d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4602858
    Commit-Queue: Michael Lippautz <[email protected]>
    Reviewed-by: Michael Lippautz <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#88447}

Refs: v8/v8@1a782f6
PR-URL: nodejs#48523
Fixes: nodejs#25382
Fixes: nodejs#14917
Refs: nodejs/performance#93
Refs: nodejs/performance#89
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Speed up child_process.spawn by enabling the new V8 build flag which
makes fork/exec faster.

Here are the results of running the existing benchmark. Note that this
optimization helps more for applications with larger heaps, so this is
somewhat of an underestimate of the real world performance benefits.

```console
$ ./node benchmark/compare.js --runs 15 \
        --new ./node \
        --old ~/node-v20/out/Release/node \
        --filter params child_process > cpr
$ node-benchmark-compare cpr
                                 confidence improvement  (***)
methodName='exec' n=1000                ***     60.84 % ±5.43%
methodName='execFile' n=1000            ***     53.72 % ±3.33%
methodName='execFileSync' n=1000        ***      9.10 % ±0.84%
methodName='execSync' n=1000            ***     10.44 % ±0.97%
methodName='spawn' n=1000               ***     53.10 % ±2.90%
methodName='spawnSync' n=1000           ***      8.64 % ±1.22%

  0.01 false positives, when considering a 0.1% risk acceptance (***)
```

Fixes: nodejs#25382
Fixes: nodejs#14917
Refs: nodejs/performance#93
Refs: nodejs/performance#89
PR-URL: nodejs#48523
Refs: v8/v8@1a782f6
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. v8 engine Issues and PRs related to the V8 dependency. labels Oct 9, 2023
Copy link
Contributor

@kvakil kvakil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me

@kvakil kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member

targos commented Nov 26, 2023

Landed in a98addb...7f68e14

targos pushed a commit that referenced this pull request Nov 26, 2023
Original commit message:

    [base] add build flag to use MADV_DONTFORK

    Embedders like Node.js and Electron expose fork(2)/execve(2) to their
    users. Unfortunately when the V8 heap is very large, these APIs become
    rather slow on Linux, due to the kernel needing to do all the
    bookkeeping for the forked process (in clone's dup_mmap and execve's
    exec_mmap). Of course, this is useless because the forked child thread
    will never actually need to access the V8 heap.

    Add a new build flag v8_enable_private_mapping_fork_optimization which
    marks all pages allocated by OS::Allocate as MADV_DONTFORK. This
    improves the performance of Node.js's fork/execve combination by 10x on
    a 600 MB heap.

    Fixed: v8:7381
    Change-Id: Ib649f774d4a932b41886313ce89acc369923699d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4602858
    Commit-Queue: Michael Lippautz <[email protected]>
    Reviewed-by: Michael Lippautz <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#88447}

Refs: v8/v8@1a782f6
PR-URL: #48523
Backport-PR-URL: #50098
Fixes: #25382
Fixes: #14917
Refs: nodejs/performance#93
Refs: nodejs/performance#89
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
@targos targos closed this Nov 26, 2023
targos pushed a commit that referenced this pull request Nov 26, 2023
Speed up child_process.spawn by enabling the new V8 build flag which
makes fork/exec faster.

Here are the results of running the existing benchmark. Note that this
optimization helps more for applications with larger heaps, so this is
somewhat of an underestimate of the real world performance benefits.

```console
$ ./node benchmark/compare.js --runs 15 \
        --new ./node \
        --old ~/node-v20/out/Release/node \
        --filter params child_process > cpr
$ node-benchmark-compare cpr
                                 confidence improvement  (***)
methodName='exec' n=1000                ***     60.84 % ±5.43%
methodName='execFile' n=1000            ***     53.72 % ±3.33%
methodName='execFileSync' n=1000        ***      9.10 % ±0.84%
methodName='execSync' n=1000            ***     10.44 % ±0.97%
methodName='spawn' n=1000               ***     53.10 % ±2.90%
methodName='spawnSync' n=1000           ***      8.64 % ±1.22%

  0.01 false positives, when considering a 0.1% risk acceptance (***)
```

Fixes: #25382
Fixes: #14917
Refs: nodejs/performance#93
Refs: nodejs/performance#89
PR-URL: #48523
Backport-PR-URL: #50098
Refs: v8/v8@1a782f6
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
@aduh95 aduh95 deleted the backport-fork-improvements branch January 12, 2024 11:31
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Original commit message:

    [base] add build flag to use MADV_DONTFORK

    Embedders like Node.js and Electron expose fork(2)/execve(2) to their
    users. Unfortunately when the V8 heap is very large, these APIs become
    rather slow on Linux, due to the kernel needing to do all the
    bookkeeping for the forked process (in clone's dup_mmap and execve's
    exec_mmap). Of course, this is useless because the forked child thread
    will never actually need to access the V8 heap.

    Add a new build flag v8_enable_private_mapping_fork_optimization which
    marks all pages allocated by OS::Allocate as MADV_DONTFORK. This
    improves the performance of Node.js's fork/execve combination by 10x on
    a 600 MB heap.

    Fixed: v8:7381
    Change-Id: Ib649f774d4a932b41886313ce89acc369923699d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4602858
    Commit-Queue: Michael Lippautz <[email protected]>
    Reviewed-by: Michael Lippautz <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#88447}

Refs: v8/v8@1a782f6
PR-URL: nodejs/node#48523
Backport-PR-URL: nodejs/node#50098
Fixes: nodejs/node#25382
Fixes: nodejs/node#14917
Refs: nodejs/performance#93
Refs: nodejs/performance#89
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Speed up child_process.spawn by enabling the new V8 build flag which
makes fork/exec faster.

Here are the results of running the existing benchmark. Note that this
optimization helps more for applications with larger heaps, so this is
somewhat of an underestimate of the real world performance benefits.

```console
$ ./node benchmark/compare.js --runs 15 \
        --new ./node \
        --old ~/node-v20/out/Release/node \
        --filter params child_process > cpr
$ node-benchmark-compare cpr
                                 confidence improvement  (***)
methodName='exec' n=1000                ***     60.84 % ±5.43%
methodName='execFile' n=1000            ***     53.72 % ±3.33%
methodName='execFileSync' n=1000        ***      9.10 % ±0.84%
methodName='execSync' n=1000            ***     10.44 % ±0.97%
methodName='spawn' n=1000               ***     53.10 % ±2.90%
methodName='spawnSync' n=1000           ***      8.64 % ±1.22%

  0.01 false positives, when considering a 0.1% risk acceptance (***)
```

Fixes: nodejs/node#25382
Fixes: nodejs/node#14917
Refs: nodejs/performance#93
Refs: nodejs/performance#89
PR-URL: nodejs/node#48523
Backport-PR-URL: nodejs/node#50098
Refs: v8/v8@1a782f6
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Original commit message:

    [base] add build flag to use MADV_DONTFORK

    Embedders like Node.js and Electron expose fork(2)/execve(2) to their
    users. Unfortunately when the V8 heap is very large, these APIs become
    rather slow on Linux, due to the kernel needing to do all the
    bookkeeping for the forked process (in clone's dup_mmap and execve's
    exec_mmap). Of course, this is useless because the forked child thread
    will never actually need to access the V8 heap.

    Add a new build flag v8_enable_private_mapping_fork_optimization which
    marks all pages allocated by OS::Allocate as MADV_DONTFORK. This
    improves the performance of Node.js's fork/execve combination by 10x on
    a 600 MB heap.

    Fixed: v8:7381
    Change-Id: Ib649f774d4a932b41886313ce89acc369923699d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4602858
    Commit-Queue: Michael Lippautz <[email protected]>
    Reviewed-by: Michael Lippautz <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#88447}

Refs: v8/v8@1a782f6
PR-URL: nodejs/node#48523
Backport-PR-URL: nodejs/node#50098
Fixes: nodejs/node#25382
Fixes: nodejs/node#14917
Refs: nodejs/performance#93
Refs: nodejs/performance#89
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Speed up child_process.spawn by enabling the new V8 build flag which
makes fork/exec faster.

Here are the results of running the existing benchmark. Note that this
optimization helps more for applications with larger heaps, so this is
somewhat of an underestimate of the real world performance benefits.

```console
$ ./node benchmark/compare.js --runs 15 \
        --new ./node \
        --old ~/node-v20/out/Release/node \
        --filter params child_process > cpr
$ node-benchmark-compare cpr
                                 confidence improvement  (***)
methodName='exec' n=1000                ***     60.84 % ±5.43%
methodName='execFile' n=1000            ***     53.72 % ±3.33%
methodName='execFileSync' n=1000        ***      9.10 % ±0.84%
methodName='execSync' n=1000            ***     10.44 % ±0.97%
methodName='spawn' n=1000               ***     53.10 % ±2.90%
methodName='spawnSync' n=1000           ***      8.64 % ±1.22%

  0.01 false positives, when considering a 0.1% risk acceptance (***)
```

Fixes: nodejs/node#25382
Fixes: nodejs/node#14917
Refs: nodejs/performance#93
Refs: nodejs/performance#89
PR-URL: nodejs/node#48523
Backport-PR-URL: nodejs/node#50098
Refs: v8/v8@1a782f6
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants